The stranded-attach warning repeats where the run ends (#614) - #625
The stranded-attach warning repeats where the run ends (#614)#625philcunliffe wants to merge 10 commits into
Conversation
Re-running `hyp init` and unchecking a previously attached client regenerates a config without that client's adapter while the client's own settings still route through the HypAware gateway. The finale only attaches, and the action reconciler's reverse lane undoes only config-named (org/central) keys, so a wizard attach on the local layer is never reversed: capture stops and the client's requests can fail. Picking only OpenTelemetry composes no gateway at all, and every still-attached client points at a dead port after the finale's restart. The finale now names the clients it left stranded (attach marker on disk, not picked, enabled by neither the written config nor the central layer) and prints the `hyp detach --client <name>` that clears each. It does not detach: rewriting a client's settings file is not what unchecking a picker row asked for, and that call is the maintainer's to make (LLP 0185). `hyp status` gains the matching `client_attached_not_configured` diagnostic as the after-the-fact backstop, gated to solo hosts because on a joined host that shape is a reconciler pass that has not run yet. The org/central attach markers are untouched throughout. Co-Authored-By: Claude <noreply@anthropic.com>
…604) Three defects in the #604 fix, found in neutral review. An unreadable local config emptied the active-plugin set for a reason that has nothing to do with what the operator enabled, and the new `client_attached_not_configured` branch read that as "nothing enables this adapter": `hyp status` answered a parse failure with a `hyp detach` for every attached client, stacked on top of the `config_unreadable` error that is the actual repair. The branch now stands down when the local layer is present but did not parse. `findAttachedNotConfiguredClients` counted a plugin entry with `enabled: false` as configured, while `hyp status` builds the same set from `activePlugins`, which skips it. A switched-off adapter collects exactly as little as an absent one, so the two surfaces disagreed about the same file. LLP 0185 records both gates, and the forward-refs the corpus convention wants land on the docs it extends: LLP 0180 (the finale's attach lane) and LLP 0086 (the client drift diagnostic family). The status test's `@ref` carried an empty gloss; it now says what it tests. Tests: an unreadable local config draws no detach advice; a disabled plugin entry strands its client; and the central-layer suppression LLP 0185 #scope promises for the finale, which had no coverage on the walkthrough side. Both new gates fail without the src change. npm test: 3386 tests / 3385 pass / 0 fail / 1 skipped. npm run typecheck clean. Smokes status_diagnostics and walkthrough_picker_to_first_query ok. Co-Authored-By: Claude <noreply@anthropic.com>
…ad unfiltered (#604) The status gate stands down on a local layer that is present but does not parse, and deliberately does not stand down when there is no config at all: nothing collects then, so a marker still on disk really is stranded. That boundary (`errorKind !== 'config_missing'`, not `!ok`) had no test, so widening the gate would have dropped the case silently. Adds one. `readCentralPluginNames` is deliberately not filtered by `enabled`, unlike the local set the caller builds: LLP 0185 #scope settles the org lane on the name alone, and filtering would hand the operator a detach for an org-owned client. Says so at the read, and scopes #not-configured-means-not-active to the local layer so the two sections cannot be read against each other. Co-Authored-By: Claude <noreply@anthropic.com>
The finale names the clients this run left attached but no longer collects, before the daemon restart (LLP 0185). `runInitWizard` then writes the run summary, a first look that is roughly sixty lines of real query output, and on the team path the privacy narration, none of which pause, so on a real terminal the warning is gone by the time setup ends. It is buried worst where it is the only signal there is: on a managed host `hyp status`'s mirror diagnostic is gated to hosts with no central layer. The wizard now repeats it, short, after the first look and before the privacy narration. The repeat is the caller's rather than the shared `writeWalkthroughRunSummary`'s, because only the caller knows whether anything came between: `runPickerWalkthrough` writes a short summary and stops, so it keeps its single print instead of showing the same paragraph twice within a few lines of itself. LLP 0188 records the print contract and carries the forward-ref on LLP 0185. Co-Authored-By: Claude <noreply@anthropic.com>
The gate was `firstLookRan || pathway === 'team'`. A `pathway` is only ever resolved on an interactive run, so a team run that is neither cancelled nor a dry run already has `firstLookRan`; the disjunct added exactly the runs where the first look did *not* run. Those are the runs that wrote nothing between the finale and here, so on a team run cancelled at the backfill consent the repeat landed three lines under the finale's own full warning, which is the same double print the shared run summary was rejected for. The gate is now `firstLookRan` alone. LLP 0188 #when says so and says why, and a new test pins the cancelled team run (fails on the old gate). Co-Authored-By: Claude <noreply@anthropic.com>
Neutral review, round 1 of 2Verdict: approve after the one fix below, which is pushed. Reviewed as Finding 1 (medium, fixed): the
|
…mpting `firstLookRan` says the closing block was attempted. It is documented to degrade to a silent skip rather than fail a finished install (LLP 0135 #first-look), so an unregistered `ai_gateway_messages` dataset, an unreadable cache, or a render that throws leaves an attended run that wrote nothing between the finale's warning and the closing repeat. Probed against the real orchestrator with no overview dataset: stdout was the config-write line and the repeat, with the finale's own six-line warning directly above it in production. That is the same-screen double print LLP 0188 says no path produces, reached through the first look's designed degradation instead of through the pathway disjunct round 1 removed. Gate on the result: `runWizardFirstLook` already returns `shown`, which is exactly "the block reached the screen". LLP 0188 §when restated to match (the condition is the closing sequence having *written* something), and the `**Extends:**` header folded into `Related:`, which is how the corpus's other 167 docs name what they extend. Co-Authored-By: Claude <noreply@anthropic.com>
Neutral review, round 2 of 2Verdict: approve after the one fix below, which is pushed ( Finding 1 (low, fixed):
|
PR #608 was squash-merged as 6ab0d99, so this branch's copies of its commits conflicted with the same content under new SHAs. Every conflicted hunk that was #608's takes master; the branch keeps only its own work (LLP 0188, writeAttachedNotConfiguredReminder, the wizard's firstLookShown gated repeat, and its tests). Co-Authored-By: Claude <noreply@anthropic.com>
|
Neutral triage on What I was checkingThe task was to re-verify, with the same skepticism that caught the first two Conflict resolution: clean
|
|
Decision on the fix shape: the second option - measure "wrote something" directly. Have Include the new orchestrator-level test exercising the slow-skip branch ( |
|
Re-engaging with your guidance - taking the second option, measure "wrote something" directly: Also taking the two specifics: a new orchestrator-level test for the slow-skip branch ( The stuck label is removed. Neutral re-runs the rung at the current head on its next tick. |
Maintainer decision on the fix shape: measure "wrote something" directly.
`runWizardFirstLook` now writes through a counter and returns `wrote`
beside `shown`, so the step reports for itself whether it put text on the
screen. The wizard's closing stranded-attach repeat gates on that.
`shown` answers "did the block render", which is not the question the
gate asks. The `slow` skip renders no block and still writes two lines
saying so, and those lines bury the finale's warning exactly as a full
render would: on a managed host, where LLP 0185 #status-backstop gates
`hyp status`'s mirror diagnostic off, that print was the only signal the
user was ever going to get. Inferring the gate from a neighbouring fact
has now broken on the no-dataset, error and slow paths in turn; a
measured value closes the class, because a skip reason added later is
counted by the same writable.
- `FirstLookResult` = `FirstLookOutcome & { wrote: boolean }`, attached
at a single exit so no branch can omit it.
- New orchestrator test: an attended run whose first look skips slowly
still repeats the warning. It fails on the `shown` gate (`not ok 22`,
`# fail 1`) and passes on the measured one. `firstLookBudgetMs` is a
test seam so it does not wait out the real 8s budget.
- The first-look unit tests now pin `wrote` per branch: true for the
render, the partial render and the slow skip, false for no-dataset and
the error skips.
- The comment claiming "a first look that skipped itself wrote nothing"
is replaced by the real contract, and LLP 0188 #when is restated to
say the condition is measured output.
`npm test`: 3413 tests, 3412 pass, 0 fail, 1 skipped. `npm run
typecheck`: clean.
Co-Authored-By: Claude <noreply@anthropic.com>
|
Applied your decision on What changed
export async function runWizardFirstLook({ runner, stdout: target, ... }) {
let wrote = false
const stdout = { write(chunk) { wrote = true; return target.write(chunk) } }
const outcome = await withSpan(...)
return { ...outcome, wrote }
}Three properties fall out of that shape, and they are what make this different from the last three fixes:
The gate reads it ( The false comment is gone ( LLP 0188 §when is restated to match: the condition is the closing sequence having written something, measured rather than inferred, with "the first look ran" named as too wide and "the block rendered" as too narrow. A The new test and its evidence
Before and after, same test file, only the gate expression differing: One failure, and it is the new test. The wizard exposes The first-look unit tests now pin Numbers
The carried nit is unchanged and still deliberate: Pushed to |
Conflicts resolved keeping both intents. The closing repeat still gates on what the first look *measured* it wrote (`firstLookResult.wrote`), never on the inferred `shown`, and `runWizardFirstLook` keeps its single exit. - src/core/cli/wizard/index.js: import block takes both sides' symbols (`writeAttachedNotConfiguredReminder` from this branch, the express-gate and TUI imports from master). `LOGIN_ORG_SELECTION_MESSAGE` is dropped: master's login lane returns its outcome (LLP 0179), so nothing reads it. The first look now has one result variable, `firstLookResult`, which both the closing repeat (`.wrote`) and master's first ask (`firstLookHadRows`) read. - src/core/cli/wizard/types.d.ts: `FirstLookOutcome`/`FirstLookResult` split kept alongside master's `FirstAskLauncher`/`FirstAskResult`/`WizardSyncNow*`; `firstLookBudgetMs` kept alongside `firstAsk`/`syncNow`. - `firstLookHadRows` takes `FirstLookOutcome`, not `FirstLookResult`: it answers from what the step found, and `wrote` says nothing about rows. - test/core/cli/wizard/first-look.test.js: master's two-section assertion (LLP 0198 #wizard-sections) with this branch's `wrote: true`. - test/core/cli/wizard/index.test.js: both new suites kept whole. LLP number collision: master landed LLP 0188 (enrolled default sync), so this branch's decision is renumbered 0188 -> 0230, the first free number above the highest claimed on any remote branch (LLP 0156 #renumber). Every inbound reference is swept: LLP 0185's Extended-by lines, and the `#when` / `#repeat-at-the-end` @refs in walkthrough.js, wizard/first_look.js, wizard/index.js, wizard/types.d.ts and the three test files.
|
Neutral triage on The maintainer's decision, verified applied at this headAll verification below was done independently in a fresh worktree at
The two flagged judgement calls
The one inconsistency the sweep left behind: two comments added by this PR still say "sixty lines" ( The renumbering (0188 to 0230), verified
Everything else
Follow-up issue: #779. Not marking ready and not merging, per the rung's bounds. |
Root cause
runPickerFinaleprints the stranded-attach warning and returns. What happensnext depends entirely on which entry point called it, and nothing in the code
accounted for that:
runPickerWalkthroughwrites its short run summary and stops. The warning iswithin a handful of lines of the end.
runInitWizardwrites the run summary, thenrunWizardFirstLook(a block ofreal query output, sized by LLP 0198 §wizard-sections), then on the team path
the privacy narration. Nothing pauses between them, so on a real terminal the
warning has scrolled away by the time
hyp initfinishes, and a user who doesnot scroll back never reads it.
writeWalkthroughRunSummarynever readfinaleSummary.attachedNotConfigured,which is what the issue reports, but that is the symptom rather than the cause:
the cause is that the finale's placement is correct for the daemon restart and
wrong for the end of the run.
It is buried worst exactly where it matters most. On a managed host
hyp status'smirror diagnostic (
client_attached_not_configured) is gated to hosts with nocentral layer (LLP 0185 #status-backstop), so the finale's print is the only
signal a wizard-attached client the reconfigure run left unpicked ever gets. A
joined host has no backstop to scroll back to.
The fix
The finale's print stays exactly where LLP 0185 put it, after the config
write and before the daemon restart.
runInitWizardadditionally writes a shortrepeat after the first look and before the privacy narration, naming the same
clients and the same
hyp detach --client <name>lines.writeAttachedNotConfiguredReminder(new export,src/core/cli/walkthrough.js) is the compact form: names plus one repaircommand each, with the same dry-run tag rule. The finale's own
writeAttachedNotConfiguredWarning, which explains the consequence in full,stays private and unchanged.
narratePrivacyIfTeamPath, so the privacy narration isstill the wizard's last words (LLP 0135, LLP 0100/0101). A short narration is a
reminder that survives on one screen; a screenful of query output is not, and
that is the difference this turns on.
runPickerWalkthroughdoes not repeat. It writes nothing substantial after thefinale, so it keeps its single print.
The gate: measured output, not an inferred branch
runWizardFirstLooknow reports whether it put anything on the screen(
src/core/cli/wizard/first_look.js). The writable its body writes through is acounter in front of the caller's, and the function has a single exit that
attaches the count:
Three properties fall out of that shape:
so no branch can print without being counted;
stdoutis the only writable in scope inside the body, so a branch added laterwrites through the counter by construction rather than by remembering to;
wroteonit.
wroteis set before delegating to the caller'swrite: a write that throwspart-way (EPIPE on a closed pipe) may already have emitted, and the safe error is
an extra reminder rather than a lost one.
The type splits accordingly:
FirstLookOutcomekeeps the branch shapes(
shown,reason, row counts) exactly as they were, andFirstLookResult = FirstLookOutcome & { wrote: boolean }.firstLookHadRowstakes the outcome half only, since
wrotesays nothing about whether the cachehas rows.
The gate in
src/core/cli/wizard/index.jsis thenstranded.length > 0 && firstLookResult?.wrote === true. Why each of thealternatives is wrong, which is the part that took three attempts:
firstLookRanis too wide. The step is documented to degrade rather thanfail a finished install (LLP 0135 #first-look), and an unregistered dataset, an
unreadable cache or a render that throws leave it having run and written
nothing. Nothing was buried, so nothing should be repeated.
shownis too narrow. Theslowskip renders no block and still writes twolines saying so, which buries the finale's print exactly as a full render
would.
pathway === 'team'is wrong rather than merely imprecise. Apathwayisonly ever resolved on an interactive run, so a team run that is neither
cancelled nor a dry run has already run the first look.
|| pathway === 'team'would therefore widen the gate to exactly the runs where the first look did
not run (cancelled at the backfill consent, or a dry run), every one of which
wrote nothing in between, so the repeat would land a few lines under the print
it repeats.
firstLookRanstill gates whether the step runs at all; only the repeat'scondition reads
wrote.The double-print hazard, and how it is avoided
Round 2 of #608's review flagged that echoing
attachedNotConfiguredfromwriteWalkthroughRunSummarywould print the warning twice within a few lines ofitself on the
runPickerWalkthroughpath, where the summary follows the finalewith nothing in between. That function is shared by both entry points, so the
naive echo is wrong twice over: it double-prints on one path and still lands
before the first look on the other, which is the complaint it was meant to fix.
So
writeWalkthroughRunSummaryis untouched. The repeat belongs to the callerthat buried the first print, because only the caller knows whether anything
came between. No path prints the warning twice on one screen.
This is a print-contract change to an Accepted LLP, so it is recorded as an
extension rather than an edit: LLP 0230 (new, Accepted) with an
Extended-by:forward-ref and an inline note added to LLP 0185§status-backstop. LLP 0185's decision itself is unchanged.
Ground truth
test/core/cli/wizard/index.test.jscarries five gate tests, four of which pinthat nothing changes:
runInitWizard: an attended run repeats the stranded-attach warning after the first look(the feature).runInitWizard: an attended run whose first look skips slowly still repeats the stranded-attach warning- team pathway,attachedNotConfigured: ['codex'], afirst-look runner that never resolves, driving the real
runWizardFirstLookthrough the real orchestrator into the
reason: 'slow'branch. It asserts theblock did not render, that the repeat ran anyway, and that it still sits
between the skip's lines and the privacy narration. This is the test that
discriminates the shipped gate: reverting it to
firstLookResult?.shown === truefails this one and only this one.runInitWizard: a scripted run does not repeat the stranded-attach warning.runInitWizard: a run cancelled at the finale does not repeat the stranded-attach warning.runInitWizard: an attended run whose first look skips itself does not repeat the stranded-attach warning(the silentno-dataset/errordegradations).test/core/cli/wizard/first-look.test.jspinswroteper branch rather thanleaving it implicit:
truefor a full render, a partial render and the slowskip;
falsefor no-dataset, no-runner and the two error skips, withstdout.text()asserted empty alongside. The EPIPE test pinswrote: true, forthe reason above.
test/core/walkthrough-unpicked-attached-clients.test.js's existingthe finale names a still-attached client the new config no longer collectsnow countsoccurrences: exactly one
hyp detach --client codexand oneStill attached, no longer collectedon therunPickerWalkthroughpath. That assertion fails underthe naive shared-summary echo.
The wizard exposes
firstLookBudgetMsbeside the existingfirstLookrunneroverride, as a test seam, so an orchestrator-level test can reach the slow branch
without waiting out the real budget.
At head
8cddf08:npm test`# tests 4089 / # pass 4088 / # fail 0 /skipped 1
;npm run typecheck` clean; CI green on all nine checks.Deferred to #779
Two comment-only leftovers, substance unaffected: two comments added by this PR
still say "sixty lines" of first-look output where LLP 0230 was correctly
softened to cite LLP 0198 §wizard-sections for the sizing
(
src/core/cli/walkthrough.js,test/core/cli/wizard/index.test.js); andwriteAttachedNotConfiguredReminder'sdryRunargument is provablyfalseatits only call site, since a measured write implies the first look ran, which
implies not a dry run. It is kept as a parameter deliberately, carried from
review round 2, and recorded on #779 for findability rather than for action.
Fixes #614